ChildProcess: account for a system error when launching a process #66
Conversation
Hi-Angel
force-pushed
the
account-for-sys-errors
branch
3 times, most recently
from
f68dff5 to
f0f0f73
Compare
|
Thanks! CI fails seems unrelated, perhaps |
Hi-Angel
force-pushed
the
account-for-sys-errors
branch
from
f0f0f73 to
d8e18be
Compare
|
Managed to build it locally, and yes, in I fixed it in a separate commit, please tell if you want it to be sent in a separate PR. |
Hi-Angel
force-pushed
the
account-for-sys-errors
branch
from
d8e18be to
54c8016
Compare
Hi-Angel
force-pushed
the
account-for-sys-errors
branch
from
54c8016 to
a95fb6e
Compare
|
I am not exactly sure what
I tried moving I mean, the only other possible change that I see is making entire |
|
Ah yeah, |
src/Node/ChildProcess.purs
Outdated
| _, _ -> if isJust $ toMaybe r.error | ||
| then BySysError | ||
| else unsafeCrashWith $ "Impossible: `spawnSync` child process neither exited nor was killed." |
There was a problem hiding this comment.
| _, _ -> if isJust $ toMaybe r.error | |
| then BySysError | |
| else unsafeCrashWith $ "Impossible: `spawnSync` child process neither exited nor was killed." | |
| _, _ -> | |
| if isJust (toMaybe r.error) then | |
| BySysError | |
| else | |
| unsafeCrashWith "Impossible: `spawnSync` child process neither exited nor was killed." |
There was a problem hiding this comment.
Or option 2:
| _, _ -> if isJust $ toMaybe r.error | |
| then BySysError | |
| else unsafeCrashWith $ "Impossible: `spawnSync` child process neither exited nor was killed." | |
| _, _ | |
| | isJust (toMaybe r.error) -> BySysError | |
| | otherwise -> unsafeCrashWith "Impossible: `spawnSync` child process neither exited nor was killed." |
There was a problem hiding this comment.
Oh, I like this one! Let me try this
There was a problem hiding this comment.
Ah sorry, didn't see this message until now 😄 I think either way is fine though stylistically, and they should compile to pretty much the same JS.
There was a problem hiding this comment.
It's okay and thank you!
When a non-existing command is attempted to run, both status and signal will be null, which leads to ChildProcess module crashing. Obviously, this is incorrect behavior. The problem is more general than that though: any system error that would result in failing to run a process would result in the module crash. Fix that by checking for such case. Fixes: purescript-node#65
Hi-Angel
force-pushed
the
account-for-sys-errors
branch
from
a95fb6e to
60eec01
Compare
|
Thank you, done! |
|
Thanks! |
|
Now released as v12.0.0 - even though this was a bug fix, the introduction of |
2 participants
Description of the change
When a non-existing command is attempted to run, both status and signal will be null, which leads to ChildProcess module crashing. Obviously, this is incorrect behavior.
The problem is more general than that though: any system error that would result in failing to run a process would result in the module crash.
Fix that by checking for such case.
Fixes: #65
Checklist: